-
-
Notifications
You must be signed in to change notification settings - Fork 544
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
XWIKI-22121: Improve the registration experience #3155
base: master
Are you sure you want to change the base?
Conversation
* Added the `back to home` button. * Fixed the style of the TODO: * replace the inline styling with some CSS
* Moved the login and register buttons from the drawer to the navbar. * Updated style * Part B: moved around the first and last name fields. TODO: * C. replace the registration confirmation inline styling with some CSS
* Split the form into two sections * Added an `About you` title for the second section. TODO: * C. replace the registration confirmation inline styling with some CSS
* Split the validation messages in multiple parts - WIP TODO: * C. replace the registration confirmation inline styling with some CSS
* Split the validation messages in multiple parts - Done. TODO: * C. replace the registration confirmation inline styling with some CSS * B. Check that other uses of Livevalidation are not broken by the changes.
* Replaced the registration confirmation inline styling with some CSS * Escaped the content added in RegistrationConfig TODO: * B. Check that other uses of Livevalidation are not broken by the changes.
* Took care of a fallback for backwards compatibility
* Tried to fix the register form not submitting, WIP
* Fixed a wrong ID for the UIExtensionClass
* Fixed the bug for unexpected lack of form submission
* Fixed some codestyle
* Added comments * Removed special rule for regex type validation in the JS initialization * Updated the template for creating a validationContainer to fit loading the form with values already given
* Improved backwards compatibility (WIP)
* Updated the style of `mandatory` and `must-match` fields when valid to never show the `Ok` text. (wasn't an issue before, because the text was left empty as a mistake) * fixed booleans for validation * fixed boolean for the welcome message formatting.
* Fixed `RegisterIT`
* Fixed the base page object register and login function to fit the new position of those links.
@Sereza7 since you are working on live validation, I just saw https://jira.xwiki.org/browse/XWIKI-9797, could it be fixed by your change? |
I wasn't aware of this. Since I started working on the project, I wasn't aware that there were livevalidation for usernames or page names repeating. From what I could see when checking those livevalidations, there is nothing about this. My guess is that livevalidation is client-side, and we don't want to have the whole list of usernames or pages on the client. This could be a performance and security issue at least. Validation is done server side but I don't think livevalidation has anything to do with it. This issue should probably be closed as |
There is no reason why live validation couldn't check if the user/page already exists with a call to the server. We don't hide if a username exists or not, from an attacker's point there is no difference if this check is done after clicking submit or before as the attacker could automate both calls. |
From the video, it was not clear to me if all the password requirements are visible from the start. It's an important point for someone thinking of a password in the beginning. Also, we can reduce the image size, on my mockups I used 200px. If that's still too big, meaning the user has to scroll to see the buttons, then maybe we will change the order of elements, first the message (Welcome), then the image. |
* Fixed the username duplicate validation message * Fixed the size of the hero image on successful registration
I'll consider this issue for a BFD then 👍
The password requirements are visible from the start. On this video I used the default policy, where we ask for at least 6 characters for a password. What's not visible at the start though are error message for 2024-06-07.15-31-18.mp4
Note that my screen is halfway covered by the inspector, so the display is unusually horizontal. I just checked and there's no need to scroll down with my screen. I quickly checked with standard phone/tablet resolutions and it's okay too. The only place I saw this scrolling issue is for the CI tests, they use a square window with relatively low resolution and the buttons are just a bit too low. I improved it in 683ccd5 :
Done in 683ccd5 👍 |
* Updated the version for deprecation
* Merge conflict resolution
* Updated version number
@@ -295,7 +296,7 @@ | |||
}, | |||
'noReturn' : true | |||
}) | |||
#set($discard = $fields.add($field)) | |||
#set($discard = $aboutYouFields.add($field)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't feel accurate to put the captcha in the about you fields, also you didn't provided any screenshot for registration with captcha?
@@ -487,6 +505,16 @@ | |||
## | |||
#end## createUser Macro | |||
{{/velocity}}</content> | |||
<attachment> | |||
<filename>undraw_done_re_oak4.svg</filename> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small detail but would be better to have a name more understandable.
#if ($xcontext.user == 'XWiki.XWikiGuest' && !$xcontext.inactiveUserReference && $xwiki.hasAccessLevel('register', 'XWiki.XWikiPreferences')) | ||
<li> | ||
<a href="$xwiki.getURL('XWiki.XWikiRegister', 'register', "xredirect=$escapetool.url($xwiki.relativeRequestURL)")" id="tmRegister" rel="nofollow">$escapetool.xml($services.localization.render('register'))</a> | ||
</li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering the impact of this for private wikis, you could have a wiki entirely private in which registration is still possible no? In which case here we're loosing seeing the register link at all, isn't it?
#set($userLink = $xwiki.getUserName("$userSpace$userName")) | ||
{{info}}$message.replace('USERLINK', "{{html clean=false}}$userLink{{/html}}"){{/info}}</registrationSuccessMessage> | ||
#set($successAndLogin = $services.localization.render('core.register.successful.successandlogin')) | ||
[[image:undraw_done_re_oak4.svg||data-xwiki-image-style-alignment="center" height="50vh"]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason for not using directly some html to put in the html macro for that one?
#set($successAndLogin = $services.localization.render('core.register.successful.successandlogin')) | ||
[[image:undraw_done_re_oak4.svg||data-xwiki-image-style-alignment="center" height="50vh"]] | ||
|
||
{{html clean="true"}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clean="true" is the default AFAIK
|
||
{{html clean="true"}} | ||
<div class="registration-success-headline"> | ||
<h2>$headline </h2> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably need some escaping
@@ -19,7 +19,7 @@ | |||
## --------------------------------------------------------------------------- | |||
## Defines what server generated error messages should look like | |||
## The error message when a field is entered incorrectly | |||
#set ($failureMessageParams = {'class': 'LV_validation_message LV_invalid'}) | |||
#set ($failureMessageParams = {'class': 'LV_invalid'}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why making that change? And you're sure it doesn't impact any test?
#foreach ($regex in $field.validate.regexes) | ||
#set($extraValidationClass = 'regex-'+ $foreach.count) | ||
#set($validation = $regex) | ||
$validationContainer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly it's not easy to review all this, I really think at a point you should get rid of all that and introduce a proper java API for this.
this.options.insertAfterWhatNode.up().appendChild(validationMessageHolder); | ||
} | ||
/* We add one validation object to the list of validations for the current field. */ | ||
this.validations.push( { type: validationFunction, params: validationParamsObj || {}, messageHolder: validationMessageHolder } ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm we're suppose to try to migrate out of prototype... a bit a pity to add new code using it.
Jira URL
https://jira.xwiki.org/browse/XWIKI-22121
Changes
Description
Clarifications
Screenshots & Video
2024-06-04.13-58-09.mp4
(note that there was a small bug with the display name after the successful registration on the video, it's been fixed since this video has been recorded)
On the video, we can see, in this order:
Executed Tests
Successfully built
xwiki-platform-administration
with the quality, integration-tests and docker profiles. Builtxwiki-platform-flamingo-skin
too. The only fails left are already here on the CI (without the changes from this branch).Manual tests, see video. For backward compatibility of the livevalidation feature, I tested on the page location form (no changes to this form, but everything worked properly still). Note that it would be good to test more backward compatibility with a couple other forms, but AFAIK this is the only other use of
livevalidation_prototype.js
in XWiki standard.Expected merging strategy